Notify and surface when a remote/cloud-agent session needs input#4605
Notify and surface when a remote/cloud-agent session needs input#4605iscekic wants to merge 10 commits into
Conversation
…attention-notifications
Anchor the attention ack store on globalThis so the single-writer detail screen and the session-list readers share one instance across Expo Router route bundles, and remount the Agents list on attention-revision change after refocus so the freezeOnBlur'd list re-reads the ack store on return.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummarySince the last review, the PR added session-title threading into push notifications (a Files Reviewed (7 files changed)
Previous Review Summaries (3 snapshots, latest commit feeed49)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit feeed49)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryOnly a trivial formatting change (wrapping an arrow-function body in braces in a test file) was made since the last review; no logic changed and the previously-fixed issues remain fixed. Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file changed)
Fix these issues in Kilo Cloud Previous review (commit dc86ab9)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThree of the four previously flagged issues (metadata-fetch outside try, throwing listener aborting notification, and over-broad list remount) are fixed in the latest commits; one lower-severity suggestion remains unaddressed. Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 1602719)Status: 4 Issues Found | Recommendation: Address before merge Executive SummaryThe highest-risk issue is in Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (14 files)
Reviewed by claude-sonnet-5 · Input: 60 · Output: 11.6K · Cached: 2.4M Review guidance: REVIEW.md from base branch |
- cloud-agent-next: fetch session metadata inside the attention-push try/catch so a metadata failure is logged and swallowed as documented, never breaking the best-effort ingest path. - mobile: key the Agents list on an attention revision snapshotted at tab focus, so it remounts to re-read the ack store only on return from a just-opened session, not on any unrelated session's attention change during browsing (preserving scroll). - mobile: isolate attention-store subscribers so one throwing listener cannot prevent the rest from being notified.
|
(bot) Thanks — addressed in
|
oxlint no-confusing-void-expression forbids returning the void expression from the arrow shorthand passed to expect().
Thread the heartbeat session title from UserConnectionDO through SessionIngestDO into the notifications service so attention and session-ready push notifications lead with the session title. - packages/notifications: optional title on sendSessionReadyNotificationInputSchema. - services/notifications: add a code-point-safe title sanitizer (collapse whitespace, <=80 code points incl. ellipsis, no split surrogate) used by dispatchCloudAgentSessionPush and the ready push. Ready push prefers the heartbeat title hint, then the DB title, then the fixed 'Kilo session ready'. - services/session-ingest: forward the heartbeat title from UserConnectionDO.claimSessionReadyPush into SessionIngestDO and on to sendSessionReadyNotification. - Tests cover title composition, long-title and surrogate-pair truncation, whitespace handling, hint-vs-DB precedence, and schema strip-on-parse.
Summary
Notifies and surfaces when a Kilo session (remote CLI or cloud agent) needs user input — a push notification plus an in-app indicator in the mobile Agents session list.
Backend push triggers
REMOTE_SESSION_ATTENTION_PUSH_ENABLEDtotrue, enabling the existing remote-session attention push path for active CLI sessions.question.asked/permission.asked) and dispatches a push notification when such an event occurs on a cloud-agent-web session, gated by presence (suppressed if a client is actively viewing the session) and session/child-session identity checks.Both slices are additive and raise-only: there is no resolve/clear push, no outbox, scheduler, alarm, or migration. Push payload
statusremains'completed'for needs-input notifications, matching existing deployed-schema expectations (#4445).Session title in push notifications
Session pushes now lead with the session's own title so a notification reads as "" over the descriptive body, instead of a generic title for every session.
sanitizeTitlehelper (inservices/notifications) trims, collapses internal whitespace, truncates to at most 80 code points (Unicode-safe, appending...), and returnsnullfor empty/whitespace-only titles.sanitizeTitle(heartbeatTitle) ?? sanitizeTitle(dbTitle) ?? 'Kilo session ready'. The live heartbeat title is threaded throughUserConnectionDO→SessionIngestDO→sendSessionReadyNotification, with an optionaltitleadded to the ready-notification RPC schema in@kilocode/notifications.sanitizeTitle(session.title) ?? 'Agent session'; the needs-input body is unchanged (Kilo needs your input.).Additive and defensive: when a title is missing or empty the existing generic fallback is used, so no notification regresses.
In-app session-list indicator (mobile)
When a session's
statusisquestionorpermission, its row in the mobile Agents session list shows a soft-pulsing amber (warn-tone)StatusDotplus aNEEDS INPUTmeta label (and "needs input" in the row'saccessibilityLabel), replacing the green live dot / timestamp meta. The indicator clears when the status leaves needs-input or when the user opens the session.status/status_updated_atalready carried bycliSessionsV2.list/search/getandactiveSessions.list— no backend, tRPC, or DB changes for this surface. Web'sSessionStatusIndicatoris the production precedent.session-attention.ts: a small predicate plus an in-memory, per-raise ack store (single writer = the session detail screen mount). Acks are intentionally not persisted across app restarts, so a genuinely unanswered question honestly re-raises. The store is anchored onglobalThisso the detail-screen writer and the session-list readers share one instance across Expo Router route bundles.StatusDotgains an opt-inpulseprop (soft opacity breathe, static underuseReducedMotion); default off leaves all existing call sites unchanged.freezeOnBlurtab reliably reflects the clear-on-open ack on return.Not included on this surface: Home-screen wiring, ack persistence, and any indicator inside the session detail screen.
Testing
services/session-ingest: full suite green, typecheck, lint clean (includes heartbeat-title threading tests).services/notifications: full suite green, typecheck, lint clean (includessanitizeTitleunit tests covering trim/whitespace-collapse, 80-code-point truncation with..., Unicode surrogate-pair truncation, lone-surrogate safety, empty-title fallback, and title-lead composition for both session-ready and cloud-agent pushes).@kilocode/notifications: typecheck, lint clean (optionaltitleon the ready-notification RPC schema).services/cloud-agent-next: full suite green, typecheck, lint clean.apps/mobile:format,typecheck,lint,check:unused, and unit tests all green, including thesession-attentionpredicate/ack-store state machine.xcrun simctl push) for both payload shapes...., theKilo session readyfallback for an empty title, and a titled needs-input push (Kilo needs your input.body) — confirmed against the notifications service delivery logs. Pixel-level rendering of the iOS notification banner could not be screenshotted in the local simulator because a freshly-installed dev-client cannot be granted notification authorization non-interactively (simctl privacyhas no notifications service); this is a generic simulator environment limitation, not a gap in this change, and OS-level delivery was confirmed (willPresentNotification).question/permissionrow shows the amber pulsing dot +NEEDS INPUTlabel and "needs input" accessibility label; a non-attention control row shows neither; the indicator clears on status change and on opening the session (and re-shows on a new raise); the remote/active-only row shows and clears via a live user-connection heartbeat fixture; and the dot renders statically under Reduce Motion with no hard blink.EXPO_ACCESS_TOKEN, no deterministic interactive-prompt harness) and are documented as accepted limitations, consistent with prior related work (feat(mobile): notify when agent sessions need input #4545).